We need a strong refactoring to separate kwargs in 2 parts, the parametrsation and caching some heavy computation.
This is on my todo list since 3 years now...
Lets do it before.

We need a strong refactoring to separate kwargs in 2 parts, the parametrsation and caching some heavy computation. This is on my todo list since 3 years now... Lets do it before.

+1 on discussing this.

Sam mentioned it on the last meeting. Would be good to bring it back.

Just a quick thought on this, there is the interpolate_bad_channels class, but that takes a list of channel ids. Given the function in this PR (and in general) you might expect it to detect-and-interpolate bad channels. I wonder if it makes sense to change that to interpolate_channels() (similar to recording.remove_channels() and then have remove_bad_channels and interpolate_bad_channels as convenience functions to detect-and-remove. However, this might be a confusing change re: backwards compatibility.

Just a quick thought on this, there is the interpolate_bad_channels class, but that takes a list of channel ids. Given the function in this PR (and in general) you might expect it to detect-and-interpolate bad channels. I wonder if it makes sense to change that to interpolate_channels() (similar to recording.remove_channels() and then have remove_bad_channels and interpolate_bad_channels as convenience functions to detect-and-remove. However, this might be a confusing change re: backwards compatibility.

Yes it makes total sense @JoeZiminski

What about having two classes:

• DetectAndRemoveBadChannels (function detect_and_remove_bad_channels)
• DetectAndInterpolateBadChannel (function: detect_and_interpolate_bad_channels)

They are a bit verbose, but it binds the bad_channel_ids of the remove/interpolate part to the detection.

@chrishalcrow what do yo think?

Just a quick thought on this, there is the interpolate_bad_channels class, but that takes a list of channel ids. Given the function in this PR (and in general) you might expect it to detect-and-interpolate bad channels. I wonder if it makes sense to change that to interpolate_channels() (similar to recording.remove_channels() and then have remove_bad_channels and interpolate_bad_channels as convenience functions to detect-and-remove. However, this might be a confusing change re: backwards compatibility.

Yes it makes total sense @JoeZiminski

What about having two classes:

• DetectAndRemoveBadChannels (function detect_and_remove_bad_channels)
• DetectAndInterpolateBadChannel (function: detect_and_interpolate_bad_channels)

They are a bit verbose, but it binds the bad_channel_ids of the remove/interpolate part to the detection.

@chrishalcrow what do yo think?

I think the verboseness makes it very clear that the function is dong two things, which is great! Thanks for the suggestion @JoeZiminski

So you'd use detect_bad_channel when you're playing with parameters and getting things going, then use detect_and_remove_bad_channels once your pipeline is a bit more settled. I think it's a nice workflow.

Hello @JoeZiminski and @alejoe91 - I've added a detect_and_interpolate_bad_channels function. At the moment, this does a very similar thing to detect_and_remove_bad_channels but calls the InterpolateBadChannels class rather than ChannelSlice.

Nice! That looks great @chrishalcrow cheers this will be super useful. I had a quick look at the code and looks good, I've got a slight preference to refactor the shared code:

        # get the default parameters from `detect_bad_channels`, and update with any user-specified parameters.
        sig = signature(detect_bad_channels)
        updated_detect_bad_channels_kwargs = {k: v.default for k, v in sig.parameters.items() if k != "recording"}
        updated_detect_bad_channels_kwargs.update(detect_bad_channels_kwargs)

        if bad_channel_ids is None:
            bad_channel_ids, channel_labels = detect_bad_channels(recording=recording, **detect_bad_channels_kwargs)
        else:
            channel_labels = None

        self._main_ids = recording.get_channel_ids()

into a new function on detect_bad_channels.py which is shared between both, but not very strong on that.

Nice! That looks great @chrishalcrow cheers this will be super useful. I had a quick look at the code and looks good, I've got a slight preference to refactor the shared code:

        # get the default parameters from `detect_bad_channels`, and update with any user-specified parameters.
        sig = signature(detect_bad_channels)
        updated_detect_bad_channels_kwargs = {k: v.default for k, v in sig.parameters.items() if k != "recording"}
        updated_detect_bad_channels_kwargs.update(detect_bad_channels_kwargs)

        if bad_channel_ids is None:
            bad_channel_ids, channel_labels = detect_bad_channels(recording=recording, **detect_bad_channels_kwargs)
        else:
            channel_labels = None

        self._main_ids = recording.get_channel_ids()

into a new function on detect_bad_channels.py which is shared between both, but not very strong on that.

I've refactored (and moved) the gross updated_detect_bad_channels_kwargs code. I think the other bits are helpful to keep for understandability.

Hello, I think this is ready @alejoe91

Hello, I now really think this is ready. @alejoe91
Docs failing because of gin. None of my doc changes actually get run.

@chrishalcrow looks perfect! Just something of fin the docs and the merge conflict and ready to merge on my end